Claude Code PR Review

PR: #323 • Head: 35b9a51 • Reviewers: stack:code-review

Summary

Two RCA-agent utility bug fixes: get-build-id.ts drops the redundant user_name query param (auth is fully carried by the Basic header), and get-failed-test-id.ts removes the && node.details?.run_count guard so a node matching the requested status is collected even when run_count is 0/absent.

Review Table

Findings

• File: src/tools/rca-agent-utils/get-failed-test-id.ts:76

• Severity: Low

• Reviewer: stack:code-review

• Issue: Dropping the && node.details?.run_count guard is a plausible, intended fix (zero-run failed tests were being silently skipped), but extractFailedTestIds is non-exported and the getTestIds module is mocked in tests/tools/rcaAgent.test.ts, so neither the old nor new condition is ever exercised. No regression guard.

• Suggestion: Export extractFailedTestIds (or drive it via getTestIds with a mocked fetch returning a hierarchy fixture) and add cases: status match with run_count: 0, status match with missing details, and a non-matching status.

• File: src/tools/rca-agent-utils/get-build-id.ts:12

• Severity: Low

• Reviewer: stack:code-review

• Issue: Removing the user_name query param is safe for auth (Basic header carries username:accessKey). Residual risk only if /builds/latest used user_name as a filter/disambiguator rather than auth — removing it would then change which build is returned.

• Suggestion: Confirm with the API owner that user_name was vestigial (not a filter). No code-level blocker.

Pre-existing (not introduced here, not blocking)

• get-failed-test-id.ts:80 pushes idMatch[1] (a string) into FailedTestInfo.test_id typed number (types.ts). Follow-up.
• Both utils call Node fetch directly rather than routing through src/lib/apiClient.ts; pre-existing, no new fetch calls added.

Verdict: PASS — two reasonable bug fixes, no Critical/High issues; add test coverage for extractFailedTestIds as a follow-up.

Claude Code PR Review

PR: #323 • Head: cb612fc • Reviewers: stack:code-review

Summary

Adds a new listBuildIds RCA tool that lists up to 5 recent builds for a project + build name across all users (progressively-widening date-window walk over the project-builds endpoint), exports extractFailedTestIds and drops its run_count guard so zero-run failures are collected, reverts the earlier getBuildId change (kept user-scoped), and adds unit tests for extractFailedTestIds, listBuildIds, and listBuildIdsTool.

Review Table

Findings

• File: src/tools/rca-agent-utils/list-build-ids.ts:139 (loop at :107)

• Severity: Medium

• Reviewer: stack:code-review

• Issue: Pages are oldest-first and next_page walks forward toward newer builds; tail keeps only the last limit. If a window exceeds MAX_PAGES_PER_WINDOW (60) pages, the loop breaks mid-window and returns builds from the middle — not the newest at the window's end. The outer loop then sees collected.length >= limit and stops, silently returning stale (non-newest) builds. Only triggers if a single build name runs >~600 times inside the narrowest 2-day window, so impact is low, but it's a silent-correctness gap.

• Suggestion: If the cap is hit while has_next is still true, skip to a behavior that reaches the newest page (request a descending sort, or surface that results may be incomplete) rather than returning the middle of the window.

• File: src/tools/rca-agent.ts:100 (listBuildIdsTool catch)

• Severity: Low

• Reviewer: stack:code-review

• Issue: listBuildIdsTool catches internally and returns { isError: true } instead of re-throwing, so the outer wrapper's handleMCPError (which carries the error-path trackMCP) never fires on a business-logic failure — only the success-path trackMCP runs. Pre-existing pattern shared by the other RCA tools, perpetuated here.

• Suggestion: Let the util throw and rely solely on the outer try/catch + handleMCPError for both instrumentation paths (follow-up; not blocking).

• File: src/tools/rca-agent-utils/list-build-ids.ts:126

• Severity: Low

• Reviewer: stack:code-review

• Issue: BuildSummary types build_number/status/started_at as required, but they're read from untyped response.json() and only build.build_id is null-checked. A build missing those fields yields undefined in a non-optional field. No crash (output is JSON-stringified), but the type is dishonest.

• Suggestion: Mark the fields optional or default them when mapping.

Notes (not blocking)

• Direct fetch use rather than src/lib/apiClient.ts follows the existing rca-agent-utils convention (get-build-id.ts, get-failed-test-id.ts) — pre-existing module tech debt, not a new deviation.
• Window re-walk on widening re-fetches narrower spans, but the common active-build case resolves in the first 2-day window (1 page); acceptable and documented.
• limit is intentionally not exposed as a tool param (prompt says "up to 5", util default is 5) — correct minimal-surface choice.
• Prior review's two items are addressed: extractFailedTestIds is now exported + covered by 6 tests; the getBuildId user_name change was reverted and documented as user-scoped.

Verdict: PASS — no Critical/High issues; build, type-check, and tests pass. The page-cap silent edge case (#1) is the only correctness item worth a follow-up guard.

Claude Code PR Review

PR: #323 • Head: 531a4b8 • Reviewers: stack:code-review

Summary

Adds a new listBuildId tool that returns the latest build ID for a project+build name across all users (no user_name filter), alongside the existing user-scoped getBuildId; exports extractFailedTestIds and drops the && run_count guard so a status-matching node with run_count: 0 is still collected; clarifies getBuildId/fetchRCA prompts to state their user-scoping; adds a dedicated extractFailedTestIds test file (6 cases) and listBuildIdTool success/error tests.

Review Table

Findings

• File: src/tools/rca-agent-utils/list-build-id.ts:19

• Severity: Medium

• Reviewer: stack:code-review

• Issue: New file calls Node fetch directly, bypassing src/lib/apiClient.ts. security.md § Dependency Security: "never use fetch … in new code. All outbound requests to BrowserStack APIs go through src/lib/apiClient.ts." This skips apiClient's proxy/CA-agent config, URL validation, and centralized error handling — a user behind a BROWSERSTACK_LOCAL_OPTION proxy or custom CA would have this tool fail while others work. Capped at Medium because it copies the existing get-build-id.ts pattern rather than inventing it.

• Suggestion: Route through apiClient.get({ url, headers }); ideally refactor get-build-id.ts in the same pass, at minimum don't add a new fetch callsite.

• File: src/tools/rca-agent-utils/list-build-id.ts:32-33

• Severity: Low

• Reviewer: stack:code-review

• Issue: await response.json() is untyped (any) and return data.build_id is unchecked. A body without build_id makes the tool return text: undefined — an invalid content payload rather than a clean error.

• Suggestion: Type the response (as { build_id?: string }) and throw a clear error when build_id is missing so the catch path returns a proper isError result.

• File: src/tools/rca-agent-utils/list-build-id.ts / get-build-id.ts

• Severity: Low

• Reviewer: stack:code-review

• Issue: The two utils are byte-identical except for the single user_name query-param line; a future error-handling fix would need to be applied twice.

• Suggestion: Collapse into one util with an optional scopeToUser/userName? parameter.

• File: src/tools/rca-agent-utils/get-failed-test-id.ts:74

• Severity: Low

• Reviewer: stack:code-review

• Issue: Removing && node.details?.run_count now collects a status-matching node with run_count: 0. Presented as a regression fix and well-tested, but the original guard was deliberate — whether a 0-run node is RCA-worthy is a product assumption.

• Suggestion: One-line confirmation from the RCA owner that 0-run failed nodes should be surfaced.

• File: src/tools/rca-agent.ts (listBuildId registration) + list-build-id.ts

• Severity: Informational (product gate, not a code defect)

• Reviewer: stack:code-review

• Issue: It is unverified that omitting user_name on /ext/v1/builds/latest returns builds across all users within the authenticated account (intended same-account visibility) vs. something broader. Credential discipline is intact (caller's own creds, no process.env), so this is a backend-contract question, not a leak in this code.

• Suggestion: Confirm the endpoint contract with the Observability/Automate API owner before merge. Consider wording the prompt "across all users in your account" to remove ambiguity for the LLM.

Verdict: PASS — no Critical/High issues; auth/multi-tenant discipline, instrumentation, and tests are sound. Before merge, confirm the listBuildId backend contract with the Observability/Automate API owner (the one true gate); the fetch→apiClient switch, the unchecked build_id, and de-duplication are recommended non-blocking follow-ups.